Refactor crop generation module as subpackage to avoid single module expanding indefinitely with added features#32
Conversation
…ated as a subpackage, moved crop generator protocol to its own stand aloen module and center and point-centered crop generator functions in their respective modules under the new subpackage.
… the full FOV best effort
…ubpackage while supporting older import signatures for compatibility
MikeLippincott
left a comment
MikeLippincott
left a comment
There was a problem hiding this comment.
LGTM! Good PR. Please let me know if any of my comments/questions do not make sense.
Last thing here that should probably be added in this PR or another PR are tests for these functions/classes! Do they behave how you expect them to?
| manifest = dataset.file_state.manifest | ||
| crop_specs: Dict[int, List[Tuple[Tuple[int, int], int, int]]] = {} | ||
|
|
||
| for idx in range(len(dataset)): |
There was a problem hiding this comment.
Consider using enumerate here?
| if x <= crop_size // 2 or x >= image_width - crop_size // 2 or \ | ||
| y <= crop_size // 2 or y >= image_height - crop_size // 2: | ||
| continue # Skip points too close to the edge for a full crop | ||
|
|
There was a problem hiding this comment.
It could be nice to log at the least counts of crops that get skipped here to understand X% of crops were too close to the edge
| if crop_size is None: | ||
| raise ValueError( | ||
| "crop_size must be provided for point-centered crop generation." | ||
| ) | ||
| if mapping is None: | ||
| raise ValueError( | ||
| "mapping must be provided for point-centered crop generation." | ||
| ) |
There was a problem hiding this comment.
could you check for float/int instead?
| dataset: BaseImageDataset, | ||
| **kwargs: Any | ||
| ) -> CropMap: | ||
| pass |
There was a problem hiding this comment.
is this supposed to be empty?
I see the inhertence in the class but nothing else? Later PR potentially?
| Compute 1D tile start positions for non-overlapping tiles centered | ||
| within an image extent. | ||
|
|
||
| :param image_extent: Size of image extent along one axis (width or height). |
There was a problem hiding this comment.
Does this assume isometric X and Y
| Generate best-effort centered, non-overlapping tiling crops | ||
| for each sample in a BaseImageDataset. | ||
|
|
||
| Tiling is "best-effort" in that it uses as many full, non-overlapping | ||
| tiles of size `crop_size` as possible along each axis, then centers | ||
| the tile grid within the remaining field of view. |
There was a problem hiding this comment.
Why non-overlapping? Consider adding a single sentence on why.
2 participants
The original
src/virtual_stain_flow/datasets/ds_engine/crop_generator.pyhouses the protocol and two types of crop generating function. As more functionalities are added the single module would expand making it hard to read and maintain. So I decided to refactor the crop generation module as a sub package. Also added a new tiling crop generator.